Adding more functionality to analyze endpoint by Krish-Gandhi · Pull Request #5658 · opensearch-project/sql

Krish-Gandhi · 2026-07-27T21:53:32Z

Description

Adding cache hit analysis, cache disabling, recommendations to analyze

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Krish Gandhi <kjg2352@gmail.com>

github-actions · 2026-07-27T21:54:37Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 58bbf5e.

Path	Line	Severity	Description
core/src/main/java/org/opensearch/sql/executor/QueryService.java	1339	medium	The withCheckedArithmetic method and its helpers (isCheckableIntegerArithmetic, isCheckableLongType, findArithmeticOverflow) were deleted. This removes the protection that rewrote PLUS/MINUS/TIMES over BIGINT operands to overflow-checked variants (Math.addExact etc.), meaning integer/long arithmetic will now silently wrap on overflow instead of throwing ArithmeticException. This is a deliberate regression of an overflow guard that was previously applied to both coordinator-side and pushed-down (script) arithmetic.
core/src/main/java/org/opensearch/sql/executor/QueryService.java	284	low	boolean disableCache is hardcoded to true with the corresponding parameter commented out (// boolean disableCache,). This unconditionally bypasses the OpenSearch shard request cache for every analyzeWithCalcite call. While bypassing cache for diagnostic endpoints is plausible, the commented-out parameter suggests this was intentionally hidden from callers rather than being a deliberate API design choice, making the behavior non-obvious.
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java	227	low	A [CACHE_DEBUG] LOG.info statement logging disableRequestCache flag state and the resolved requestCache() value is commented out but left in production code. Debug log lines that expose internal request configuration details are a minor anomaly and should be removed before merging.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 1 | Low: 2

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-07-27T21:55:36Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Typo Field name 'serverity' is misspelled. Should be 'severity'. This typo propagates to the JSON API response and will require a breaking change to fix later. private final RecommendationSeverityLevel serverity; Resource Leak ThreadLocal `disableRequestCache` is set but never removed in the normal execution path when `executeWithCalcite` completes successfully. Only the error path and the latch.await() interruption path call `.remove()`. This leaks ThreadLocal state across requests in thread pools, causing cache disabling to persist incorrectly for subsequent queries on the same thread. boolean disableCache = true; // Phase 1: Execute via the exact same path as executeWithCalcite + executionEngine.execute // to get identical profile timings. Use a latch to synchronize the async callback. // Force profiling on so executeWithCalcite activates QueryProfiling. QueryContext.setProfile(true); String[] indexNames = extractIndexNames(plan); long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); if (disableCache) { CalcitePlanContext.disableRequestCache.set(true); } AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); AtomicReference<QueryProfile> profileRef = new AtomicReference<>(); AtomicReference<Exception> errorRef = new AtomicReference<>(); CountDownLatch latch = new CountDownLatch(1); executeWithCalcite( plan, queryType, null, new ResponseListener<>() { @Override public void onResponse(ExecutionEngine.QueryResponse response) { ProfileMetric formatMetric = QueryProfiling.current().getOrCreateMetric(MetricName.FORMAT); long formatStart = System.nanoTime(); int resultSize = response.getResults().size(); for (var exprValue : response.getResults()) { exprValue.tupleValue().entrySet().stream() .map(e -> e.getValue().value()) .toArray(Object[]::new); } formatMetric.set(System.nanoTime() - formatStart); profileRef.set(QueryProfiling.current().finish()); queryResponseRef.set(response); latch.countDown(); } @Override public void onFailure(Exception e) { errorRef.set(e); latch.countDown(); } }); try { latch.await(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); CalcitePlanContext.disableRequestCache.remove(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } finally { CalcitePlanContext.disableRequestCache.remove(); } Possible Issue `cacheHitsBefore` is retrieved before `disableRequestCache` is set to true (line 294 reads cache hits, line 297-299 disables cache). If the goal is to measure cache behavior with cache disabled, reading `cacheHitsBefore` before disabling the cache means the 'before' measurement may itself be a cache hit, making the comparison unreliable. If the intent is to compare with-cache vs without-cache across two executions, the current code only runs one execution (with cache disabled), so `cacheHitsAfter` will never exceed `cacheHitsBefore` and `possibleCacheHit` will always be false when `disableCache=true`, making lines 350-355 dead code in that case. long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); if (disableCache) { CalcitePlanContext.disableRequestCache.set(true); } AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); AtomicReference<QueryProfile> profileRef = new AtomicReference<>(); AtomicReference<Exception> errorRef = new AtomicReference<>(); CountDownLatch latch = new CountDownLatch(1); executeWithCalcite( plan, queryType, null, new ResponseListener<>() { @Override public void onResponse(ExecutionEngine.QueryResponse response) { ProfileMetric formatMetric = QueryProfiling.current().getOrCreateMetric(MetricName.FORMAT); long formatStart = System.nanoTime(); int resultSize = response.getResults().size(); for (var exprValue : response.getResults()) { exprValue.tupleValue().entrySet().stream() .map(e -> e.getValue().value()) .toArray(Object[]::new); } formatMetric.set(System.nanoTime() - formatStart); profileRef.set(QueryProfiling.current().finish()); queryResponseRef.set(response); latch.countDown(); } @Override public void onFailure(Exception e) { errorRef.set(e); latch.countDown(); } }); try { latch.await(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); CalcitePlanContext.disableRequestCache.remove(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } finally { CalcitePlanContext.disableRequestCache.remove(); } if (errorRef.get() != null) { listener.onFailure(errorRef.get()); return; } long cacheHitsAfter = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); boolean possibleCacheHit = !disableCache && cacheHitsBefore >= 0 && cacheHitsAfter >= 0 && cacheHitsAfter > cacheHitsBefore;

github-actions · 2026-07-27T21:56:11Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix typo in field name The field name `serverity` is misspelled and should be `severity`. This typo will propagate through the API response and could cause confusion or integration issues for API consumers. core/src/main/java/org/opensearch/sql/executor/AnalyzeResponse.java [65-73] @Data @Builder public static class Recommendation { - private final RecommendationSeverityLevel serverity; + private final RecommendationSeverityLevel severity; private final String rule; private final String message; private final String affected_node; private final String suggestion; } Suggestion importance[1-10]: 9 __ Why: The suggestion identifies a critical typo in the field name `serverity` which should be `severity`. This is a public API field that will be exposed in responses, making this a high-impact bug that could cause integration issues.	High
	Remove redundant ThreadLocal cleanup The `ThreadLocal` cleanup in the `catch` block is redundant because the `finally` block already ensures cleanup. Remove the duplicate `remove()` call from the `catch` block to avoid confusion and maintain cleaner exception handling. core/src/main/java/org/opensearch/sql/executor/QueryService.java [336-343] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - CalcitePlanContext.disableRequestCache.remove(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } finally { CalcitePlanContext.disableRequestCache.remove(); } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies redundant `ThreadLocal` cleanup in the `catch` block when the `finally` block already handles it. This is a valid code quality improvement that eliminates unnecessary duplication.	Medium
	Remove unreachable cache tracking code The `disableCache` variable is hardcoded to `true`, making the conditional logic for cache hit tracking unreachable. Either remove the dead code branches or make `disableCache` configurable via a parameter to enable cache hit detection when needed. core/src/main/java/org/opensearch/sql/executor/QueryService.java [287-298] -boolean disableCache = true; // Phase 1: Execute via the exact same path as executeWithCalcite + executionEngine.execute // to get identical profile timings. Use a latch to synchronize the async callback. // Force profiling on so executeWithCalcite activates QueryProfiling. QueryContext.setProfile(true); String[] indexNames = extractIndexNames(plan); -long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); +boolean disableCache = true; +long cacheHitsBefore = -1; -if (disableCache) { - CalcitePlanContext.disableRequestCache.set(true); -} +CalcitePlanContext.disableRequestCache.set(true); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `disableCache` is hardcoded to `true`, making the conditional logic for `cacheHitsBefore` unreachable. However, the commented parameter `// boolean disableCache` at line 278 suggests this may be intentionally disabled for now with plans to make it configurable later.	Medium
	Simplify disabled cache hit logic Since `disableCache` is hardcoded to `true`, `cacheHitsAfter` will always be `-1` and `possibleCacheHit` will always be `false`. This creates misleading logic that suggests cache hit detection is functional when it's actually disabled. Simplify by directly setting `possibleCacheHit = false`. core/src/main/java/org/opensearch/sql/executor/QueryService.java [350-355] -long cacheHitsAfter = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); -boolean possibleCacheHit = - !disableCache - && cacheHitsBefore >= 0 - && cacheHitsAfter >= 0 - && cacheHitsAfter > cacheHitsBefore; +long cacheHitsAfter = -1; +boolean possibleCacheHit = false; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that with `disableCache` hardcoded to `true`, the complex conditional logic for `possibleCacheHit` is unnecessary. However, similar to suggestion 1, this may be intentionally structured for future configurability.	Medium

Adding cache hit analysis, cache disabling, recommendations to analyze

58bbf5e

Signed-off-by: Krish Gandhi <kjg2352@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adding more functionality to analyze endpoint - #5658

Adding more functionality to analyze endpoint#5658
Krish-Gandhi wants to merge 1 commit into
opensearch-project:mainfrom
Krish-Gandhi:feature/analyze-enhancements

Krish-Gandhi commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Krish-Gandhi commented Jul 27, 2026

Description

Related Issues

Check List

Uh oh!

github-actions Bot commented Jul 27, 2026

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jul 27, 2026

PR Reviewer Guide 🔍

Uh oh!

github-actions Bot commented Jul 27, 2026

PR Code Suggestions ✨

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant